Skip to content

Refactor how we handle overflow and remove blanket Pattern impl #23580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 24, 2015
Merged

Refactor how we handle overflow and remove blanket Pattern impl #23580

merged 2 commits into from
Mar 24, 2015

Conversation

@nikomatsakis
Copy link
Contributor

See comments in the commits for reasoning

r? @aturon
cc @Kimundi

Kimundi
Kimundi reviewed Mar 21, 2015
View reviewed changes
src/libcore/str/pattern.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be the doc comment of the &str impl instead, or rather explaining that it delegates to the &str impl.

@Kimundi
Copy link
Contributor

Kimundi commented Mar 21, 2015

Apart from the doc comments, the pattern impls look fine to me.

@alexcrichton
Copy link
Member

Is it possible to still have a forwarding impl, but just for references?

impl<'a, 'b, P: Pattern<'a> + ?Sized + 'b> Pattern<'a> for &'b P {
    ...
}

@nikomatsakis
Copy link
Contributor Author

@alexcrichton no, because any type P: Fn is also P: Pattern, and if we make it so that &F is Fn, then you have two overlapping routes to derive that &P: Pattern.

@nikomatsakis nikomatsakis force-pushed the pattern-and-overflow branch from 9d68f33 to 8ff0d74 Compare March 23, 2015 20:40
@nikomatsakis
Copy link
Contributor Author

rebased, tweaked docs per @Kimundi's suggestion

@aturon
Copy link
Contributor

aturon commented Mar 23, 2015

@bors: r+ 8ff0d74

@nikomatsakis
Refactor how we handle overflow so that it is a fatal error that aborts
8bd8466 
compilation: this removes all the ungainly code that special cases
overflow so that we can ensure it propagates.
@nikomatsakis
Remove auto-deref'ing Pattern impl because it conflicts with other
76ead08 
possible blanket impls and also triggers internal overflow. Add some
special cases for common uses (&&str, &String) for now; bounds-targeting
deref coercions are probably the right longer term answer.
@nikomatsakis nikomatsakis force-pushed the pattern-and-overflow branch from 8ff0d74 to 76ead08 Compare March 23, 2015 22:05
@nikomatsakis
Copy link
Contributor Author

@bors r=aturon 7dead08

@bors
Copy link
Collaborator

bors commented Mar 23, 2015

🙀 7dead08 is not a valid commit SHA. Please try again with 76ead08.

@nikomatsakis
Copy link
Contributor Author

@bors r=aturon 76ead08

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2015
@alexcrichton
rollup merge of rust-lang#23580: nikomatsakis/pattern-and-overflow
bed7740
@bors bors merged commit 76ead08 into rust-lang:master Mar 24, 2015
@ghost
Copy link

ghost commented Mar 26, 2015

I think the overflow refactoring here might be what @aturon was referring to in the issue I reported at #23714. The overflow/failure on evaluating the Sized bound causes my ackermann.rs crate to no longer compile since I am using Cow in a rose-tree like structure here which triggers the issue.

@Munksgaard Munksgaard mentioned this pull request Apr 12, 2015
Open
@nikomatsakis nikomatsakis deleted the pattern-and-overflow branch March 30, 2016 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@aturon aturon

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nikomatsakis @Kimundi @alexcrichton @aturon @bors